Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show debug message if fee recipient from builder differs from registration #6024

Merged
merged 5 commits into from
Aug 9, 2022
Merged

Show debug message if fee recipient from builder differs from registration #6024

merged 5 commits into from
Aug 9, 2022

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented Aug 4, 2022

PR Description

Noticed that there was no fee recipient check for execution payloads from a builder. It is possible that a dishonest builder could set this to a different address. I'm not entirely sure that showing a warning and proceeding is the correct thing to do. Maybe we should fall back to the local execution engine.

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@ajsutton
Copy link
Contributor

ajsutton commented Aug 4, 2022

Ah the catch here is that it's generally expected that the builder won't set the actual fee recipient to the supplied value. They can build a block in advance of knowing who the fee recipient is (giving them more time) using their own fee recipient then just insert one last transaction into the block to transfer the payment to the requested fee recipient. It may still be worth having an option to warn in this situation but it's not a clear sign of a dishonest builder (it probably indicates a smart builder optimising things well).

@jtraglia
Copy link
Contributor Author

jtraglia commented Aug 4, 2022

Ah I understand, that makes sense. Do you want an experimental option to enable this warning? I would also tone down the severity of the message and make it shorter. I'm not sure how useful it would be now though.

@ajsutton
Copy link
Contributor

ajsutton commented Aug 5, 2022

I don't really know what we should do here. I lean towards not bothering but I'm not the MEV expert. @StefanBratanov and @benjaminion any thoughts?

@StefanBratanov
Copy link
Contributor

StefanBratanov commented Aug 5, 2022

Hey, i remember having a discussion about this when the BuilderBidValidator was implemented and we decided not to verify the fee recipient since as @ajsutton mentioned it is not a sign of a dishonest builder and builders may prefer this strategy.

I think though actually it could be useful to add this log statement as LOG.debug in BuilderBidValidatorImpl. (think it's better place to add this since you have access to the registration and the header). Also I would probably change the message to say that it is expected behavior and a small explanation.

@jtraglia jtraglia changed the title Show warning if fee recipient from builder differs from registration Show debug message if fee recipient from builder differs from registration Aug 5, 2022
@jtraglia
Copy link
Contributor Author

jtraglia commented Aug 5, 2022

@StefanBratanov thanks! I think a debug message makes more sense and I agree that BuilderBidValidatorImpl is a much better place for this. I gave it a fairly brief debug message and a large comment (mostly stolen from @ajsutton) above that block of code. If a user sees this message and wants to learn more, I hope they reference that comment.

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ajsutton ajsutton merged commit e3ba555 into Consensys:master Aug 9, 2022
@jtraglia jtraglia deleted the add-fee-recipient-warning branch August 9, 2022 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants